fix: Scorm file upload error #79
Conversation
There was a problem hiding this comment.
@talhaaslam01 Thanks for creating PR. I have a suggestion if you can look into it that would be great.
openedxscorm/scormxblock.py
Outdated
| @@ -384,7 +384,7 @@ def extract_package(self, package_file): | |||
| # the is_dir() method to verify whether a ZipInfo object points to a | |||
| # directory. | |||
| # https://docs.python.org/3.6/library/zipfile.html#zipfile.ZipInfo.is_dir | |||
| if not zipinfo.filename.endswith("/"): | |||
| if not (zipinfo.filename.endswith("/") or zipinfo.filename.endswith("\\")): | |||
There was a problem hiding this comment.
what if you use is_dir method to check if file is directory or not as mentioned in the inline comment since python 3.6+ is supported now?
# Do not unzip folders, only files. In Python 3.6 we will have access to
# the is_dir() method to verify whether a ZipInfo object points to a
# directory.
# https://docs.python.org/3.6/library/zipfile.html#zipfile.ZipInfo.is_dir
There was a problem hiding this comment.
@ziafazal, the is_dir method checks for / at the end of the filename which is the same as condition zipinfo.filename.endswith("/") currently being used.
'is_dir' method implementation:
def is_dir(self):
"""Return True if this archive member is a directory."""
return self.filename[-1] == '/'
From python docs:
ZipInfo.is_dir()
Return True if this archive member is a directory.
This uses the entry’s name: directories should always end with /.
New in version 3.6.
And, I have found this path in filenames filename: 'plugins\\plugins\\' in some of my scorm zip files.
Also, it is weird that filenames are using backslashes \\ in the path but the is_dir method is checking for / forwardslash only.
There was a problem hiding this comment.
@talhaaslam01 it appears the issue lies with archives created using Windows OS having older version of PowerShell. That underlying issue appears to be fixed in PowerShell and there is new is_dir method which addresses the issue in python. I'd suggest backporting is_dir method in our codebase until we upgrade to python 3.12.3 or greater which has a fix for windows based archives. Please add an inline TODO comment about removing the method once we upgrade to python 3.12.3 or greater.
| if not (zipinfo.filename.endswith("/") or zipinfo.filename.endswith("\\")): | |
| def is_dir(zipinfo): | |
| """Return True if this archive member is a directory.""" | |
| if zipinfo.filename.endswith('/'): | |
| return True | |
| # The ZIP format specification requires to use forward slashes | |
| # as the directory separator, but in practice some ZIP files | |
| # created on Windows can use backward slashes. For compatibility | |
| # with the extraction code which already handles this: | |
| if os.path.altsep: | |
| return zipinfo.filename.endswith((os.path.sep, os.path.altsep)) | |
| return False |
There was a problem hiding this comment.
@ziafazal, I have updated the code.
talhaaslam01
force-pushed
the
talha/upload-error
branch
3 times, most recently
from
bca5fe8 to
0debe81
Compare
openedxscorm/scormxblock.py
Outdated
| @@ -42,6 +42,7 @@ def _(text): | |||
|
|
|||
|
|
|||
| logger = logging.getLogger(__name__) | |||
| os.path.altsep = '\\' | |||
There was a problem hiding this comment.
Can you define this as constant instead of overriding python library's internal property?
There was a problem hiding this comment.
Updated to OS_PATH_ALT_SEP.
talhaaslam01
force-pushed
the
talha/upload-error
branch
2 times, most recently
from
bcc6376 to
422227b
Compare
talhaaslam01
force-pushed
the
talha/upload-error
branch
from
422227b to
b13dfdd
Compare
Description
Some of my Scorm files (client's courses) are not being uploaded because, during extraction, some of the filenames are ending with
\\double backslash. But, the code ignores only the filenames ending with/forward slash.